Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java 17 instanceof pattern matching for modules #82341

Merged
merged 3 commits into from
Jan 10, 2022

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Jan 7, 2022

Switch to Java 17 instanceof pattern matching for folders build-conventions through modules.

@astefan astefan added the :Core/Infra/Core Core issues without another label label Jan 7, 2022
@astefan astefan marked this pull request as ready for review January 7, 2022 23:02
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 7, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except a few smaller things, no need for another round.

Thanks for improving this!

@@ -1209,8 +1209,7 @@ private void sync(Path sourceRoot, Path destinationRoot, BiConsumer<Path, Path>
}
});
} catch (UncheckedIOException e) {
if (e.getCause() instanceof NoSuchFileException) {
NoSuchFileException cause = (NoSuchFileException) e.getCause();
if (e.getCause()instanceof NoSuchFileException cause) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing space

Suggested change
if (e.getCause()instanceof NoSuchFileException cause) {
if (e.getCause() instanceof NoSuchFileException cause) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henningandersen unfortunately, that whitespace missing there is the result of spotless, because of a bug. https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
Even if it was fixed on the Eclipse side, we need to wait for Spotless to upgrade. I think diffplug/spotless#1046 this is the issue we are waiting for to be fixed.

@@ -58,8 +58,8 @@ public MapXContentParser(

@Override
protected boolean doBooleanValue() throws IOException {
if (iterator != null && iterator.currentValue() instanceof Boolean) {
return (Boolean) iterator.currentValue();
if (iterator != null && iterator.currentValue()instanceof Boolean aBoolean) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space

Suggested change
if (iterator != null && iterator.currentValue()instanceof Boolean aBoolean) {
if (iterator != null && iterator.currentValue() instanceof Boolean aBoolean) {

@@ -216,8 +216,8 @@ public NumberType numberType() throws IOException {

@Override
public byte[] binaryValue() throws IOException {
if (iterator != null && iterator.currentValue() instanceof byte[]) {
return (byte[]) iterator.currentValue();
if (iterator != null && iterator.currentValue()instanceof byte[] bytes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space:

Suggested change
if (iterator != null && iterator.currentValue()instanceof byte[] bytes) {
if (iterator != null && iterator.currentValue() instanceof byte[] bytes) {

@@ -621,22 +621,19 @@ public void visitStringConcatenation(StringConcatenationNode irStringConcatenati
int j = i;
irRightNode.visit(this, (e) -> irStringConcatenationNode.getArgumentNodes().set(j + 1, e));

if (irLeftNode instanceof ConstantNode && irRightNode instanceof ConstantNode) {
ConstantNode irConstantNode = (ConstantNode) irLeftNode;
if (irLeftNode instanceof ConstantNode irConstantNode && irRightNode instanceof ConstantNode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems slightly inconsistently used here, in that the cast to ConstantNode seems unnecessary. I would prefer to either cast/pattern match both left and right nodes or none of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't particularly like this block of code changes either. Seems too compact and the code too similar between the conditional branches to make it easy to read. I've reverted the changes. Even if the instanceof pattern matching is not used, the readability of the code is slightly better this way.

irConstantNode.attachDecoration(
new IRDConstant(
"" + irConstantNode.getDecorationValue(IRDConstant.class) + irRightNode.getDecorationValue(IRDConstant.class)
)
);
irConstantNode.attachDecoration(new IRDExpressionType(String.class));
irStringConcatenationNode.getArgumentNodes().remove(i + 1);
} else if (irLeftNode instanceof NullNode && irRightNode instanceof ConstantNode) {
ConstantNode irConstantNode = (ConstantNode) irRightNode;
} else if (irLeftNode instanceof NullNode && irRightNode instanceof ConstantNode irConstantNode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, seems simpler to just use irRightNode in the block? Also easier to read.

irConstantNode.attachDecoration(new IRDConstant("" + null + irRightNode.getDecorationValue(IRDConstant.class)));
irConstantNode.attachDecoration(new IRDExpressionType(String.class));
irStringConcatenationNode.getArgumentNodes().remove(i);
} else if (irLeftNode instanceof ConstantNode && irRightNode instanceof NullNode) {
ConstantNode irConstantNode = (ConstantNode) irLeftNode;
} else if (irLeftNode instanceof ConstantNode irConstantNode && irRightNode instanceof NullNode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@astefan astefan merged commit 35a79bc into elastic:master Jan 10, 2022
@astefan astefan deleted the java17/instanceof/modules branch January 10, 2022 08:43
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.0

astefan added a commit to astefan/elasticsearch that referenced this pull request Jan 10, 2022
Switch to Java 17 instanceof pattern matching for folders build-conventions through modules
elasticsearchmachine pushed a commit that referenced this pull request Jan 10, 2022
Switch to Java 17 instanceof pattern matching for folders build-conventions through modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue >refactoring Team:Core/Infra Meta label for core/infra team v8.0.0-rc2 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants